Skip to content

Add reference for Gueymard 93 relative airmass model #2424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 4, 2025

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Mar 31, 2025

  • Closes Reference for Gueymard 1993 air mass equation incorrect #2390
  • I am familiar with the contributing guidelines
  • [ ] Tests added
  • [ ] Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • [ ] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

@cwhanse cwhanse added this to the v0.12.1 milestone Mar 31, 2025
Copy link
Contributor

@RDaxini RDaxini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're working on the references, let's add some underscores? This should hyperlink the numbers to their respective citation in the list at the bottom of the page.
(I can only suggest changes to lines that were changed or next to those that were changes, but I think we should update the entire page in this PR)

@@ -153,7 +153,7 @@ def get_relative_airmass(zenith, model='kastenyoung1989'):
requires true sun zenith
* 'kastenyoung1989' (default) - See reference [3] -
requires apparent sun zenith
* 'gueymard1993' - See reference [4] -
* 'gueymard1993' - See references [4] and [9] -
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 'gueymard1993' - See references [4] and [9] -
* 'gueymard1993' - See references [4]_ and [9]_ -

@@ -174,6 +174,8 @@ def get_relative_airmass(zenith, model='kastenyoung1989'):
other models use true (not refraction-adjusted) zenith angle. Apparent
zenith angles should be calculated at sea level.

Comparison among several models is reported in [10].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Comparison among several models is reported in [10].
Comparison among several models is reported in [10]_.

@@ -153,7 +153,7 @@ def get_relative_airmass(zenith, model='kastenyoung1989'):
requires true sun zenith
* 'kastenyoung1989' (default) - See reference [3] -
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 'kastenyoung1989' (default) - See reference [3] -
* 'kastenyoung1989' (default) - See reference [3]_ -

@@ -153,7 +153,7 @@ def get_relative_airmass(zenith, model='kastenyoung1989'):
requires true sun zenith
* 'kastenyoung1989' (default) - See reference [3] -
requires apparent sun zenith
* 'gueymard1993' - See reference [4] -
* 'gueymard1993' - See references [4] and [9] -
requires apparent sun zenith
* 'young1994' - See reference [5] -
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 'young1994' - See reference [5] -
* 'young1994' - See reference [5]_ -

Comment on lines 150 to 156
* 'kasten1966' - See [1]_ - requires apparent sun zenith
* 'youngirvine1967' - See [2]_ - requires true sun zenith
* 'kastenyoung1989' (default) - See [3]_ - requires apparent sun zenith
* 'gueymard1993' - See [4]_, [9]_ - requires apparent sun zenith
* 'young1994' - See [5]_ - requires true sun zenith
* 'pickering2002' - See [6]_ - requires apparent sun zenith
* 'gueymard2003' - See [7]_, [8]_ - requires apparent sun zenith
Copy link
Contributor

@RDaxini RDaxini Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking, but should we adjust the order? [9] -> [5] and so on. I missed this earlier, just noticed it now.
(reference list would need to change accordingly)

Suggested change
* 'kasten1966' - See [1]_ - requires apparent sun zenith
* 'youngirvine1967' - See [2]_ - requires true sun zenith
* 'kastenyoung1989' (default) - See [3]_ - requires apparent sun zenith
* 'gueymard1993' - See [4]_, [9]_ - requires apparent sun zenith
* 'young1994' - See [5]_ - requires true sun zenith
* 'pickering2002' - See [6]_ - requires apparent sun zenith
* 'gueymard2003' - See [7]_, [8]_ - requires apparent sun zenith
* 'kasten1966' - See [1]_ - requires apparent sun zenith
* 'youngirvine1967' - See [2]_ - requires true sun zenith
* 'kastenyoung1989' (default) - See [3]_ - requires apparent sun zenith
* 'gueymard1993' - See [4]_, [5]_ - requires apparent sun zenith
* 'young1994' - See [6]_ - requires true sun zenith
* 'pickering2002' - See [7]_ - requires apparent sun zenith
* 'gueymard2003' - See [8]_, [9]_ - requires apparent sun zenith

Copy link
Contributor

@echedey-ls echedey-ls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


Documentation
~~~~~~~~~~~~~
* Add a supporting reference to :py:func:`pvlib.atmosphere.get_relative_airmass` (:issue:`2390`, :pull:`2424`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Add a supporting reference to :py:func:`pvlib.atmosphere.get_relative_airmass` (:issue:`2390`, :pull:`2424`)
* Add a supporting bibliographic reference to model ``gueymard1993`` in
:py:func:`pvlib.atmosphere.get_relative_airmass` (:issue:`2390`, :pull:`2424`)

Clarification for users out of context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought just "reference" was okay without "bibliographic". Adding the model ``gueymard1993`` in seems reasonable though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I sometimes think of code references, but given this piece of description is in the Docs section, you are completely right.

In: Polo, J., Martín-Pomares, L., Sanfilippo, A. (eds) Solar Resources
Mapping. Green Energy and Technology. Springer, Cham.
:doi:`10.1007/978-3-319-97484-2_5`

.. [9] Matthew J. Reno, Clifford W. Hansen and Joshua S. Stein, "Global
.. [10] Matthew J. Reno, Clifford W. Hansen and Joshua S. Stein, "Global
Horizontal Irradiance Clear Sky Models: Implementation and Analysis"
Sandia Report, (2012).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Sandia Report, (2012).
Sandia Report, (2012).
:doi:`10.2172/1039404`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't add DOIs for the other references but here they are:
1: 11681/5671
2: 10.1086/110366
3: 10.1364/AO.28.004735
4: 10.1016/0038-092X(93)90074-X

@cwhanse
Copy link
Member Author

cwhanse commented Apr 2, 2025

Thanks @RDaxini @AdamRJensen your comments motivated me to update the formatting to the IEEE convention.


.. [2] A. T. Young and W. M. Irvine, "Multicolor Photoelectric
Photometry of the Brighter Planets," The Astronomical Journal, vol.
72, pp. 945-950, 1967.
:doi:`10.2172/110366`
Copy link
Contributor

@RDaxini RDaxini Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... this doi did not work for me. Try:

Suggested change
:doi:`10.2172/110366`
:doi:`10.1086/110366`


.. [7] Keith A. Pickering. "The Ancient Star Catalog". DIO 12:1, 20,
.. [7] Keith A. Pickering, "The Southern Limits of the Ancient Star Catalog
and the Commentary fo Hipparchos," DIO, vol. 12, pp. 3-27, Sept. 2002.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fo needs to be of (for some reason it won't let me suggest it)

Could consider adding the url: http://dioi.org/jc01.pdf

Copy link
Contributor

@RDaxini RDaxini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kandersolar
Copy link
Member

Thanks everyone for the reviews!

@kandersolar kandersolar merged commit 0812192 into pvlib:main Apr 4, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reference for Gueymard 1993 air mass equation incorrect
5 participants